Skip to content

Incorrect isinstance(..., 'str') check, test _pyproject_toml #413

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Incorrect isinstance(..., 'str') check, test _pyproject_toml #413

wants to merge 6 commits into from
+100 −1

Conversation

maurycy
Copy link
Contributor

@maurycy maurycy commented Sep 13, 2025 •
edited
Loading

'str' is incorrect there:

>>> isinstance('meow', 'str')
Traceback (most recent call last):
  File "<python-input-0>", line 1, in <module>
    isinstance('meow', 'str')
    ~~~~~~~~~~^^^^^^^^^^^^^^^
TypeError: isinstance() arg 2 must be a type, a tuple of types, or a union
>>> isinstance('meow', str)
True

Reference:

@maurycy maurycy changed the title Incorrect isinstance(readme, 'str') check Incorrect isinstance(..., 'str') check Sep 13, 2025
@maurycy
Copy link
Contributor Author

maurycy commented Sep 14, 2025 •
edited
Loading

If I find more similarly obvious issues, I will create one PR for them to avoid flooding the PR here.

@hugovk
Copy link
Member

hugovk commented Sep 15, 2025

Looks like we're missing tests for this bit of code. Would it be possible to add something?

@maurycy
Copy link
Contributor Author

maurycy commented Sep 20, 2025 •
edited
Loading

@hugovk

Looks like we're missing tests for this bit of code. Would it be possible to add something?

Definitely! Thank you for spotting this.

What do you think about bd5e2d2? It fails on the main branch.

Personally, I'm not a huge fan of this, as it's importing an _internal module. It seems to be the rule, though:

If you like this stub, I can extend it with more tests, especially given that #109 wasn't a particularly lucky PR:

More generally, one could wonder if the project should reimplement PEP 621. Are we OK with exploring https://github.com/pypa/pyproject-metadata? I wish we uv in Python projects. ;-)

More importantly: how much of this code was ever executed, given no bug reports over the past 3 years? Similarly to how we wondered after running ruff check. Notably: ValuError also happened in pyperformance/_pyproject_toml.py.

maurycy
maurycy commented Sep 20, 2025
View reviewed changes
pyperformance/tests/test_pyproject_toml.py
def test_parse_project_with_readme_string(self):
with tempfile.TemporaryDirectory() as tmpdir:
tmp_path = pathlib.Path(tmpdir)
toml_content = """
Copy link
Contributor Author

@maurycy maurycy Sep 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://docs.python.org/3/library/textwrap.html#textwrap.dedent

🤔

@maurycy maurycy changed the title Incorrect isinstance(..., 'str') check Incorrect isinstance(..., 'str') check, test _pyproject_toml Sep 22, 2025
@hugovk
Copy link
Member

hugovk commented Sep 30, 2025

Personally, I'm not a huge fan of this, as it's importing an _internal module. It seems to be the rule, though:

I think that's fine here.

More generally, one could wonder if the project should reimplement PEP 621. Are we OK with exploring pypa/pyproject-metadata?

Yeah, given the comment at the top of _pyproject_toml.py, I reckon using pypa/pyproject-metadata is ok:

pyperformance/pyperformance/_pyproject_toml.py

Lines 1 to 2 in 553aa5d

# This module should be replaced with the equivalent functionality
# in the PyPI "packaging" package (once it's added there).

hugovk
hugovk approved these changes Sep 30, 2025
View reviewed changes
Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hugovk hugovk hugovk approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@maurycy @hugovk @AA-Turner